Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Session::RemoveContent() #1179

Merged
merged 12 commits into from
Feb 19, 2025
Merged

Conversation

simue
Copy link
Contributor

@simue simue commented Feb 13, 2025

Closes #1171

@simue simue force-pushed the fix/reset-session-content branch from 405d702 to 6dc4338 Compare February 14, 2025 11:13
Copy link
Member

@COM8 COM8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simue perfect. Thanks!
Could you please add a bit of in line documentation for your two new functions like I'm doing it here:
#1168

Also please add a few sentences of documentation about your feature here:
https://github.com/libcpr/docs

@COM8 COM8 added this to the CPR 1.11.x milestone Feb 16, 2025
@simue
Copy link
Contributor Author

simue commented Feb 16, 2025

Now, that I am trying to document this, I am hesitating with the resulting interface:
1.per default, Session keeps the previous payload around.
2.user has to explicitly clear the internal state (if he knows the library well enough)

Do you know if people are actually using session to send the same payload multiple times? Because if they are not, I´d rather call RemoveContent() after every request that carries a body.

@simue
Copy link
Contributor Author

simue commented Feb 17, 2025

MultiperformPerformTests.MultiperformMixedMethodsPerformTest seems to be flaky - did you see it have issues before?

@COM8
Copy link
Member

COM8 commented Feb 17, 2025

Now, that I am trying to document this, I am hesitating with the resulting interface: 1.per default, Session keeps the previous payload around. 2.user has to explicitly clear the internal state (if he knows the library well enough)

Do you know if people are actually using session to send the same payload multiple times? Because if they are not, I´d rather call RemoveContent() after every request that carries a body.

Ah, yes :D
Yes, people are actually using this behaviour (and I'm as well for some of my API requests). Because of this and since it would be a really drastic change in how cpr::Session works I'd keep it as it is.

This always has been a bit of a discussion point but we decided to go this way. Even if it's the more intuitive way that in reality might almost always require more code to clear the content before reusing.

@COM8
Copy link
Member

COM8 commented Feb 17, 2025

MultiperformPerformTests.MultiperformMixedMethodsPerformTest seems to be flaky - did you see it have issues before?

Yes, especially on macOS. This is a limitation of the mock server we are using for our tests.

Copy link
Member

@COM8 COM8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 2 Minor things regarding your comments and the we are ready to go!

@simue simue requested a review from COM8 February 18, 2025 20:02
@COM8 COM8 merged commit df19eb5 into libcpr:master Feb 19, 2025
42 of 44 checks passed
COM8 added a commit that referenced this pull request Feb 19, 2025
@COM8
Copy link
Member

COM8 commented Feb 19, 2025

Approved, merged and is now part of the 1.11.x branch. I will try to make a new release over the next weekend.

@simue
Copy link
Contributor Author

simue commented Feb 20, 2025

thank you very much for the good collaboration!

@simue simue deleted the fix/reset-session-content branch February 20, 2025 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using sessions, the body/payload/multipart of the previous request is sent if not replaced.
2 participants